Reset repo feature #211

rmunn · 2023-07-27T09:58:16Z

WIP on reset repo feature. Currently have UI and backend, but UI not yet hooked up to backend.

related to #155

github-actions · 2023-07-27T09:59:05Z

UI unit Tests

1 tests ±0 1 ✔️ ±0 0s ⏱️ ±0s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 320c22e. ± Comparison against base commit b2c778b.

♻️ This comment has been updated with latest results.

github-actions · 2023-07-27T09:59:44Z

Unit Tests

0 tests - 8 0 ✔️ - 8 0s ⏱️ -7s
0 suites - 3 0 💤 ±0
0 files - 1 0 ❌ ±0

Results for commit c7a22e2. ± Comparison against base commit 2ce46b3.

♻️ This comment has been updated with latest results.

rmunn · 2023-07-27T09:59:48Z

Still to do: actually downloading the zip file. Also, figuring out where on the project page to place the reset button; I just arbitrarily put it above the Mercurial history, but I think we want a red "Danger" area with buttons like this.

rmunn · 2023-07-27T10:05:08Z

One UI issue that bugs me:

Shouldn't the checkbox be on the left?

rmunn · 2023-07-27T14:24:36Z

Also to do: need to verify whether the .zip file produced by .NET Core will handle UTF-8 filenames correctly, because audio files in FLEx projects will often have names in the target language (which will often be a non-Roman script). If the functions built into .NET Core can't handle UTF-8 filenames correctly, we'll have to use 7zip (found in the 7z command on Linux) to do the zipping.

rmunn · 2023-07-27T14:39:33Z

Looks like the ZipFile.CreateFromDirectory method does indeed handle UTF-8 filenames correctly: I made a test project and created a zip file with a filename full of non-Roman characters, and the resulting zip file had the correct filename in UTF-8 inside (confirmed by taking a hex dump of the first hundred or so bytes of the .zip file). So there's no need to reach for an external tool.

We used 7zip in Language Forge because at the time, Windows 7 was the most recent version of Windows and it still had a bug handling non-Roman filenames in zip files. But that bug has been fixed in Windows 10, and as long as we're using recent versions of .NET, there's no problem using the built-in zip file handlers on Unicode filenames.

rmunn · 2023-07-27T20:33:20Z

Note that once we're using .NET 8, we'll be able to use ZipFile.CreateFromDirectory without creating a temporary file. Which will reduce some code complexity and potentially save some time.

alex-larkin · 2023-08-02T15:47:49Z

Yes, I think that the left side would be more intuitive, and a better design choice.

Modal dialog in UI currently does nothing, I'll hook it up next

frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte

myieye · 2023-08-11T13:53:12Z

We've agreed that we'll initially backup deleted (also repos that have been deleted as part of reseting them) into a "fake repo" called "deleted". Repos that are explicitly deleted are suffixed with the deletion timestamp i.e. {project-code}__{deleted-at}. In order to differentiate between those and repos that have been reset, reset repos should do something slightly different such as {project-code}__{deleted-at}__reset.

The HgService has a SoftDeleteRepo(string code, string deletedRepoSuffix) method ready for this.
See: #212 (comment)

myieye · 2023-08-12T16:18:05Z

One UI issue that bugs me:

Shouldn't the checkbox be on the left?

@rmunn yeah, it appears to me that the vast majority of forms with a checkbox have the checkbox on the left with the text immediately to the right of it (rather than the text being justified to the far right of the form). I think that would be an improvement for our Checkbox component. The only other place we use it (I think) is the project creation page. Perhaps make sure it looks ok there as well. Great work!!

Checkbox now looks correct, and label has enough spacing

It will now actually reset the project.

Now that we're using the accent color, it clashes in both color and look. The button's distinct color is enough.

Fixes merge conflicts

Now that the code is wrapped in {#if project}, we don't need to write `project?.code` because we know it's defined at that point.

rmunn · 2023-08-24T07:50:01Z

I believe I've addressed all of @myieye's review comments. @hahn-kev, would you like to review it again since @myieye is out of the office at the moment? It's been a couple of weeks since it was looked at, so I think another look might be good before we merge it.

hahn-kev

It looks great Robin, thanks.
There's some minor changes I'd like to the API used to reset the project. I'd also like to see us use the FormModal and then have the user type in reset before we actually let the run the reset. Could also verify they clicked the download link too actually.

backend/LexBoxApi/Controllers/ProjectController.cs

frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte

frontend/src/routes/(authenticated)/project/[project_code]/ResetProjectModal.svelte

frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte

backend/LexBoxApi/Services/HgService.cs

github-actions · 2023-09-01T07:27:43Z

C# Unit Tests

8 tests 8 ✔️ 10s ⏱️
3 suites 0 💤
1 files 0 ❌

Results for commit 320c22e.

♻️ This comment has been updated with latest results.

We'll use an object, not a string, as the return value, to make it easier to add other fields later if desired.

Now that we're reusing the SoftDelete method, which doesn't return anything, there's no backupPath to return. It was never all that meaningful to the end user anyway, so just get rid of it and simply return an empty HTTP 200 to indicate success.

#211 (comment)

Also move the business logic inside the modal, rather than in the page, similarly to how AddProjectMember and ChangeMemberRole work.

If the repo directory cannot be found, HgService.BackupRepo will return null for the backup file path, which will turn into a 404 in the ProjectController. We'll want to detect that 404 in the UI when the user clicks on the "Backup Project" button and disallow submitting the form if that happens.

rmunn · 2023-09-07T08:37:15Z

Merge conflicts resolved (by merging develop into the branch), all review discussions (so far) resolved as well. Time for a re-review in case the merge did something weird, then this will finally be ready to merge!

myieye

Lots of nit picks 😉

frontend/src/lib/i18n/locales/en.json

frontend/src/routes/(authenticated)/project/[project_code]/ResetProjectModal.svelte

backend/LexBoxApi/Services/HgService.cs

frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte

frontend/src/routes/(authenticated)/project/[project_code]/ResetProjectModal.svelte

frontend/src/lib/i18n/locales/en.json

frontend/src/routes/(authenticated)/project/[project_code]/ResetProjectModal.svelte

myieye

@rmunn looks good! 👍🏽
I implemented and pushed all my feedback.
You should probably look it over.

hahn-kev

Looks good to me. I created #275 to track some features that we decided not to implement yet.

rmunn linked an issue Jul 27, 2023 that may be closed by this pull request

project reset history feature #155

Closed

rmunn added 2 commits August 3, 2023 13:19

First steps towards the reset-project feature

4967527

Modal dialog in UI currently does nothing, I'll hook it up next

Modal now submits properly

647e126

rmunn force-pushed the feature/reset-repo branch from 2881b4c to 647e126 Compare August 3, 2023 06:19

myieye reviewed Aug 3, 2023

View reviewed changes

frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte Outdated Show resolved Hide resolved

myieye reviewed Aug 3, 2023

View reviewed changes

frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte Outdated Show resolved Hide resolved

myieye reviewed Aug 3, 2023

View reviewed changes

frontend/src/routes/(authenticated)/project/[project_code]/+page.svelte Outdated Show resolved Hide resolved

rmunn added 2 commits August 15, 2023 15:33

Better look for reset project modal

62b655c

Checkbox now looks correct, and label has enough spacing

Hook up project-reset button

ef30507

It will now actually reset the project.

rmunn changed the title ~~Reset repo feature (WIP)~~ Reset repo feature Aug 18, 2023

rmunn marked this pull request as ready for review August 18, 2023 08:27

rmunn added 7 commits August 23, 2023 16:11

Improve look of reset-project button

cd92c84

Remove border around reset-project button

aa22686

Now that we're using the accent color, it clashes in both color and look. The button's distinct color is enough.

Merge branch 'develop' into feature/reset-repo

b881004

Fixes merge conflicts

Use new SoftDeleteRepo in ResetRepo code

b493926

Add translated text for project-reset notification

e3cb57c

Use AdminContent component instead of #if isAdmin

9c6f92e

No need for ?. when we know the project is defined

d22bfe6

Now that the code is wrapped in {#if project}, we don't need to write `project?.code` because we know it's defined at that point.

rmunn requested a review from myieye August 24, 2023 07:50

rmunn requested a review from hahn-kev August 24, 2023 07:50

Use non-nullable _project to silence linter

c7a22e2

hahn-kev requested changes Aug 24, 2023

View reviewed changes

Address some review comments

0e4d7a4

rmunn added 7 commits September 1, 2023 16:09

Refactor output of reset project API

faa4046

We'll use an object, not a string, as the return value, to make it easier to add other fields later if desired.

Merge branch 'develop' into feature/reset-repo

4dd151f

Remove ResetProjectByAdminOutput class

b159d10

Now that we're reusing the SoftDelete method, which doesn't return anything, there's no backupPath to return. It was never all that meaningful to the end user anyway, so just get rid of it and simply return an empty HTTP 200 to indicate success.

Edit Checkbox component to have label on right

92786bc

#211 (comment)

Rewrite ResetProjectModal to use FormDialog

f7fb3ca

Also move the business logic inside the modal, rather than in the page, similarly to how AddProjectMember and ChangeMemberRole work.

No need for target="_blank" in the backup link

dc339ce

rmunn requested a review from hahn-kev September 7, 2023 08:37

myieye requested changes Sep 7, 2023

View reviewed changes

myieye added 2 commits September 7, 2023 16:55

Review fixups

e7502f1

Merge remote-tracking branch 'origin/develop' into feature/reset-repo

320c22e

myieye force-pushed the feature/reset-repo branch from 588f109 to 320c22e Compare September 7, 2023 10:12

myieye approved these changes Sep 7, 2023

View reviewed changes

hahn-kev approved these changes Sep 11, 2023

View reviewed changes

hahn-kev merged commit 13845e3 into develop Sep 11, 2023
11 checks passed

myieye deleted the feature/reset-repo branch October 5, 2023 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset repo feature #211

Reset repo feature #211

rmunn commented Jul 27, 2023 •

edited by hahn-kev

Loading

github-actions bot commented Jul 27, 2023 •

edited

Loading

github-actions bot commented Jul 27, 2023 •

edited

Loading

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

alex-larkin commented Aug 2, 2023

myieye commented Aug 11, 2023 •

edited

Loading

myieye commented Aug 12, 2023

rmunn commented Aug 24, 2023

hahn-kev left a comment

github-actions bot commented Sep 1, 2023 •

edited

Loading

rmunn commented Sep 7, 2023

myieye left a comment

myieye left a comment

hahn-kev left a comment

Reset repo feature #211

Reset repo feature #211

Conversation

rmunn commented Jul 27, 2023 • edited by hahn-kev Loading

github-actions bot commented Jul 27, 2023 • edited Loading

UI unit Tests

github-actions bot commented Jul 27, 2023 • edited Loading

Unit Tests

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

rmunn commented Jul 27, 2023

alex-larkin commented Aug 2, 2023

myieye commented Aug 11, 2023 • edited Loading

myieye commented Aug 12, 2023

rmunn commented Aug 24, 2023

hahn-kev left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 1, 2023 • edited Loading

C# Unit Tests

rmunn commented Sep 7, 2023

myieye left a comment

Choose a reason for hiding this comment

myieye left a comment

Choose a reason for hiding this comment

hahn-kev left a comment

Choose a reason for hiding this comment

rmunn commented Jul 27, 2023 •

edited by hahn-kev

Loading

github-actions bot commented Jul 27, 2023 •

edited

Loading

github-actions bot commented Jul 27, 2023 •

edited

Loading

myieye commented Aug 11, 2023 •

edited

Loading

github-actions bot commented Sep 1, 2023 •

edited

Loading